-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Network Manager #116
Network Manager #116
Conversation
The Helm README.md appears to be out-of-date. Please, run
Here it is an excerpt of the diff:78c78%0A< Autogenerated from chart metadata using [helm-docs v1.14.2](https://github.com/norwoodj/helm-docs/releases/v1.14.2)%0A---%0A> Autogenerated from chart metadata using [helm-docs v1.11.0](https://github.com/norwoodj/helm-docs/releases/v1.11.0) |
2 similar comments
The Helm README.md appears to be out-of-date. Please, run
Here it is an excerpt of the diff:78c78%0A< Autogenerated from chart metadata using [helm-docs v1.14.2](https://github.com/norwoodj/helm-docs/releases/v1.14.2)%0A---%0A> Autogenerated from chart metadata using [helm-docs v1.11.0](https://github.com/norwoodj/helm-docs/releases/v1.11.0) |
The Helm README.md appears to be out-of-date. Please, run
Here it is an excerpt of the diff:78c78%0A< Autogenerated from chart metadata using [helm-docs v1.14.2](https://github.com/norwoodj/helm-docs/releases/v1.14.2)%0A---%0A> Autogenerated from chart metadata using [helm-docs v1.11.0](https://github.com/norwoodj/helm-docs/releases/v1.11.0) |
@LorenzoMoro Thanks for this PR. |
I'm completely fine with KnownClusters, since this CRD will work also for Catalogue-fetched Clusters from other domains, which can't be defined Neighbors. |
Thanks @LorenzoMoro for your work! I think the CRD should be rewritten a bit with an appropriate struct for the Spec and one for the Status, but for this initial PR and feature implementation may be good anyway. One last step before proceeding with the PR is to squash your commits into a single commit, as suggested in our Contribution Guide. Thanks again for your contribution, really appreciated! |
Thanks @andreacv98! The port follows the IP address, so as long as it is set after the IP address in "networkManager.configMaps.nodeIdentity.ip" at installation time (lines 213 and 221 in installation.sh), it is advertised via multicast by the sender clusters and accordingly detected by the receiving clusters.
We know the CRD isn't formatted as necessary, but it will as soon as we implement the aging control, thus working as a fully compliant k8s controller (and by that time the Status will be necessary, right now it's stateless).
Sure, we'll update the naming as suggested by @frisso, squash and update the PR! |
Thanks @LorenzoMoro for the PR! Overall, it looks like a good job! I share what Fulvio says on the CRD name; let's remember that a I also share what Andrea is saying on the overall structure of the API: let's keep it uniform so we can keep the readability of our code base. Additionally, we should also think how we support dynamic discoveries: currently the network manager execute at the beginning but it does not reconcile ( Only a small request for whom want to test it: could you provide an example yaml of this new CRD under Only waiting for modifications and squashes for approving and merging. Thanks! |
e2ef0d4
to
68f57bc
Compare
We've updated the Network Manager to implement also the aging check, so now it's full working. Also format issues in the CRD etc. have been solved. It's still structured as a Controller, though the Reconciler is never triggered right now since the functions we're running are synchronous and the controller pattern works best with asynchronous workflows: we kept the Controller structure to easily implement future functionalities. |
e9e8317
to
dd875f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @LorenzoMoro, thanks for the PR!
I only reported a few comments on the network interface management.
A hard-coded value has been used (eth1) but I would investigate whether it's possible to find a better way.
However, I'm not sure this could be an easy task and we can possibly postpone for a future investigation considering that the network manager is an optional component by now.
Thanks,
Francesco
dd875f1
to
992e0b3
Compare
Hi all, eth1 has been parametrized, it was pretty simple since it already came from an annotation. |
One week.
Network Manager first implementation added, multicast discovery is working and provider IPs are fetched from "Cluster CR" instead of ConfigMap. TODO: implementation of CR aging control for clusters which disconnected from LAN and other cosmetic stuff, but the feature seems to work.